Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add http2 directive instead of listen option #1579

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

C24-AK
Copy link
Contributor

@C24-AK C24-AK commented Oct 17, 2023

Pull Request (PR) description

This PR is meant to create the http2 directive: http://nginx.org/en/docs/http/ngx_http_v2_module.html#http2

This Pull Request (PR) fixes the following issues

Fixes following warning in nginx:
nginx: [warn] the "listen ... http2" directive is deprecated, use the "http2" directive instead.

@kenyon
Copy link
Member

kenyon commented Oct 17, 2023

The tests need to be updated to match this change.

@C24-AK
Copy link
Contributor Author

C24-AK commented Oct 18, 2023

I can't test this with these containers, since nginx is getting pulled from nginx repo which does not have the latest nginx package. Locally we are testing with latest version, since it is required for this PR.
@kenyon how do I proceed from here?

@kenyon
Copy link
Member

kenyon commented Oct 19, 2023

You can specify an nginx version in an acceptance test's puppet code, just like in regular puppet code. Since the http2 directive doesn't exist in older versions, I think you have to have some logic that only uses it if the nginx version is 1.25.1 or newer.

@C24-AK
Copy link
Contributor Author

C24-AK commented Oct 24, 2023

@kenyon Hey, can you tell me why archlinux is the only test failing? It seems like other Pull Requests have the same problem.

@kenyon
Copy link
Member

kenyon commented Oct 24, 2023

@C24-AK Archlinux test failure is expected for now.

@C24-AK
Copy link
Contributor Author

C24-AK commented Nov 1, 2023

@kenyon Hey, what to do on following error?
This happens outside of examples:

JSON::ParserError:
unexpected token at ''
./vendor/bundle/ruby/2.7.0/gems/json-2.6.3/lib/json/common.rb:216:in parse' ./vendor/bundle/ruby/2.7.0/gems/json-2.6.3/lib/json/common.rb:216:in parse'
./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:99:in load_filters' ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:80:in block in merge_filters'
./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:79:in each' ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:79:in merge_filters'
./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:201:in run_report' ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:186:in report!'
./vendor/bundle/ruby/2.7.0/gems/voxpupuli-test-7.0.2/lib/voxpupuli/test/spec_helper.rb:21:in `block (2 levels) in <top (required)>'

@kenyon
Copy link
Member

kenyon commented Nov 1, 2023

Yeah I don't know what is causing that. Some debugging required.

@C24-AK
Copy link
Contributor Author

C24-AK commented Nov 2, 2023

Is this something I need to fix for this PR to be merged?

@smortex
Copy link
Member

smortex commented Nov 3, 2023

That looks better, thanks! (Edit: The archlinux failure seems unrelated)

Can you please combine your 40+ commits in a single one?

From your working directory:

git rebase -i origin/main # Interactively rewrite history

This will bring your editor listing all commits in your branch. Keep the first line unchanged, and replace pick with squash for all subsequent lines. Save and quit, your editor will pop again and allow you to combine all the commit messages in a single one.

Then push your updated branch:

git push -f            # Send the changes (-f is required because we re-wrote history)

@C24-AK
Copy link
Contributor Author

C24-AK commented Nov 6, 2023

Hey @smortex,

I did what you explained and it feels like all my changes are gone?
All tests are failing

Edit: I reverted it, should be fixed.

adjusted http2 checks

adding nginx version to acceptance tests

revert changes

set default version to 1.25.2

adjusted docs

revert changes

Test with repository which contains the latest version

Test with default repo source and manage repo true

Test without setting version

Test with 1.25.1

Debugging facts

Debugging facts

Debugging facts

add version to mail

fix a test for 1.14

testing

testing

Debug and tracing

set package_ensure

disable rubocop

disable rubocop

set package to altest

set package to latest

mailspec

remove tracing

set http2

set http2 directive

mailhost

mailhost tracing

revert

revert

revert

http2 tests

http2 tests

http2 tests

testing

testing

testing

http2 test

http2 test

http2 test

http2 test

Fix http2 for old versions
@C24-AK
Copy link
Contributor Author

C24-AK commented Nov 9, 2023

@smortex anything else to do or am I finished here?

@smortex
Copy link
Member

smortex commented Nov 9, 2023

@smortex anything else to do or am I finished here?

That look good, the failures seems unrelated, I tried to run the test again, maybe it will pass and if not maybe this will ring a bell to some other contributor?

I added an in-line note about old test removal, and I wonder if we can keep them for now?

@C24-AK
Copy link
Contributor Author

C24-AK commented Nov 9, 2023

@smortex I resolved your comment and added the tests

@kenyon
Copy link
Member

kenyon commented Nov 10, 2023

I just reran the failing Puppet 7 job to see if it's consistent.

@kenyon
Copy link
Member

kenyon commented Nov 10, 2023

I'm not sure how this change would be causing this error, but it seems like it is.

From https://github.com/voxpupuli/puppet-nginx/actions/runs/6809552993/job/18546775611?pr=1579#step:5:299

An error occurred in an `after(:suite)` hook.
Failure/Error: Parser.new(source, **(opts||{})).parse

JSON::ParserError:
  unexpected token at ''
# ./vendor/bundle/ruby/2.7.0/gems/json-2.6.3/lib/json/common.rb:216:in `parse'
# ./vendor/bundle/ruby/2.7.0/gems/json-2.6.3/lib/json/common.rb:216:in `parse'
# ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:99:in `load_filters'
# ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:80:in `block in merge_filters'
# ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:79:in `each'
# ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:79:in `merge_filters'
# ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:201:in `run_report'
# ./vendor/bundle/ruby/2.7.0/gems/rspec-puppet-4.0.0/lib/rspec-puppet/coverage.rb:186:in `report!'
# ./vendor/bundle/ruby/2.7.0/gems/voxpupuli-test-7.0.2/lib/voxpupuli/test/spec_helper.rb:21:in `block (2 levels) in <top (required)>'

@C24-AK
Copy link
Contributor Author

C24-AK commented Nov 10, 2023

@kenyon I am not very familiar with this. How would I fix this issue? I appreciate any help

@C24-AK
Copy link
Contributor Author

C24-AK commented Nov 13, 2023

Is this rdy to merge? @kenyon

@kenyon kenyon added the enhancement New feature or request label Nov 13, 2023
@kenyon kenyon changed the title added http2 directive instead of listen option Add http2 directive instead of listen option Nov 13, 2023
@kenyon kenyon merged commit c6ab9de into voxpupuli:master Nov 13, 2023
25 of 27 checks passed
@TheMeier TheMeier added this to the 6.0.0 milestone Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants